Add Servo integration via --browser=servo#92
Add Servo integration via --browser=servo#92ErichDonGubler merged 4 commits intoerichdongubler-mozilla:mainfrom
--browser=servo#92Conversation
This comment was marked as outdated.
This comment was marked as outdated.
acba05f to
3267b9e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
ErichDonGubler
left a comment
There was a problem hiding this comment.
LGTM with some nitpicks and issues that I've resolved with fixup! commits. LMK what you think before I merge things; I've tried to leave good notes in the commits themselves, but I've translated feedback to conversations here, too.
| const SCOPE_DIR_SERVO_PUBLIC_STR: &str = "tests/wpt/webgpu"; | ||
| const SCOPE_DIR_SERVO_PUBLIC_COMPONENTS: &[&str] = &["tests", "wpt", "webgpu"]; |
There was a problem hiding this comment.
issue(non-blocking): Hmm, I think trying to use a "public"/"private" scope model for Servo reveals some Mozilla-centricity in my thinking when I proposed TestVisibility. Looking at Servo's current layout of the tests/wpt folder, lemme make a table to demonstrate what I think current mental models are:
TestVisibility variant |
Files used in Firefox | Files Erich thinks Servo's WebGPU team uses based on this PR | Files Erich assumed Servo used before this PR |
|---|---|---|---|
Private |
testing/web-platform/mozilla/{meta,tests} (_mozilla manifest/route prefix). |
tests/wpt/webgpu/{meta,tests} (_webgpu manifest/route prefix) |
tests/wpt/mozilla/{meta,tests} (_mozilla manifest/route prefix) |
Public |
testing/web-platform/{meta,tests} (upstream manifest, no route prefix) |
Unused? | tests/wpt/{meta,tests} (upstream manifest, no route prefix) |
The reason that moz-webgpu-cts cares about distinguishing Public from Private tests really for debugging and making some of the formatting code more consistent. It seems that Servo's test runs only produce reports with WebGPU tests under the _webgpu route prefix? If that's the case, I think it'd be more sensible to make TestScope an enum again, with different models of scopes between browsers, rather than trying to impose TestVisibility on everything. That way, Servo can have a single variant for its WebGPU paths, and be done.
There was a problem hiding this comment.
I have a resolution for this with #103, which we can save for after this PR.
There was a problem hiding this comment.
What does private/public mean in this context? Are WebGPU CTS tests meant to be private or public (I considered them as public), but maybe public is meant for upstream WPT tests?
In servo _webgpu is alias to tests/wpt/webgpu/{meta,tests} which contains WebGPU CTS. An that's it, those are only WebGPU tests we have.
There was a problem hiding this comment.
@sagudev: Yep, "public" was supposed to mean "upstream WPT". 😅 I definitely wasn't using the right vocabulary; hopefully the table and the PR clarify the details here.
There was a problem hiding this comment.
Why exactly would upstream path be needed for moz-webgpu-cts? Are there any webgpu test there, or is this some kind of future-proofing for when CTS lands in upstream WPT (or moz-webgpu-cts takes over all expectation updating)?
There was a problem hiding this comment.
We now have the right filtering in place for Firefox CI. So, we don't generate reports with unrelated entries anymore. At the time, though, it was easy to work around it with git add <scope>/meta/webgpu && git restore . locally. Even now, we don't fully automate the committing of expected properties updated via moz-webgpu-cts; there's still a human that looks at things that changed. I don't foresee this changing, as we slowly validate what parts of the CTS are stable for us in Firefox.
But "public" results/files are not just skipped? They are also read and formatted, IIRC?
Sorry, I didn't expect you to be interested in details, so I've been pretty terse. 😅 You're right; report results for any tests outside <scope>/meta/webgpu will still get processed and written out by moz-webgpu-cts as with tests it's intended to be used for.
There was a problem hiding this comment.
report results for any tests outside /meta/webgpu will still get processed and written out by moz-webgpu-cts as with tests it's intended to be used for.
That's what's been confusing me the most. In servo there are a lot of "invalid" and not formatted expectations files, so that's why we cannot have moz-webgpu-cts try to load them.
There was a problem hiding this comment.
Is the Servo project actually interested in using moz-webgpu-cts in a broader scope? Or is that merely a pain point that's forced you to narrow what metadata moz-webgpu-cts is incidentally exposed to? 🤔
There was a problem hiding this comment.
Is the Servo project actually interested in using
moz-webgpu-ctsin a broader scope?
Formatting would be useful as I do not think any exists for expectations (or at least we aren't using any), but there is currently nothing planned.
Or is that merely a pain point that's forced you to narrow what metadata
moz-webgpu-ctsis incidentally exposed to? 🤔
This is exactly what happened.
There was a problem hiding this comment.
Formatting would be useful as I do not think any exists for expectations (or at least we aren't using any), but there is currently nothing planned.
I suspect that a general-purpose formatter would be very difficult to get consensus on, as opposed to the very opinionated approach here (CC @jgraham). I don't see any upstream issues in wpt or rfcs for it. However, you could use whippit now to create your own parser, and then a custom emitter like format_file to enforce things like property order and spacing. AIUI that's really the full breadth of things you can control, with the metadata language.
--browser=servo
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
132510d to
30e61d7
Compare
Split off #80 with #80 (comment) taken into account. This PR only contains minimal work to acknowledge servo's existence.
It works on servo (if also used with sagudev@dddc19d), but I do not have m-c clone to test with.